Add more print schema ordering options #2042

id-ilych · 2022-08-05T15:35:29Z

Hello 👋

I noticed that printSchema only have options to order type and directives definitions. But smaller elements like fields, and types in unions have unspecified order which on major upgrades creates quite chaotic diff in the dumped .graphql file.
I added options for sub elements so that it is more stable.

I also added default localeCompare-based implementations to orderPrintedDefinitions helper and updated affected tests.

My main two concerns:

Is it ok that I updated the orderPrintedDefinitions to give more stable output?
Is it ok not to create dedicated test for that (there are no tests for print.ts file) considering that update of the orderPrintedDefinitions is observable in other tests?

netlify · 2022-08-05T15:35:31Z

👷 Deploy request for apollo-federation-docs pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	`d25fa15`

codesandbox-ci · 2022-08-05T15:36:23Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

benweatherman · 2022-08-05T18:21:16Z

howdy @id-ilych and thanks for the PR! Would you mind updating with the latest from main (apologies, we just merged something that required updating a few of the same tests). Also, would you mind adding a note at the top of the composition-js and internals-js CHANGELOGs?

Is it ok that I updated the orderPrintedDefinitions to give more stable output?

Is it ok not to create dedicated test for that (there are no tests for print.ts file) considering that update of the orderPrintedDefinitions is observable in other tests?

Yes to both of these!

id-ilych · 2022-08-06T10:00:02Z

Thanks @benweatherman. I did as you suggested.

benweatherman · 2022-08-08T14:39:49Z

@id-ilych thanks for the updates! for some reason, our CI isn't running fully on your commits. Have you signed the apollo CLA previously? is there any chance you're using CircleCI's follow feature to follow your fork of the code?

id-ilych · 2022-08-08T16:07:39Z

Hey @benweatherman 👋 I have signed CLA on 23 Jun 2021. But I did so again, just in case. Regarding the CircleCI, I did not do any configuration. Should I?

benweatherman · 2022-08-08T18:55:24Z

No need to do any special config. Following the fork in CircleCI was the internal working theory as to why the tests weren't running. Will dig into this a bit more.

pcmanus · 2022-08-17T15:50:17Z

I'll admit that I'm on the fence about changing the current output of composition.

I'm totally on board about adding the option to order everything, and in principle I don't disagree that ordering everything can be desirable. But changing the order has very visible consequences, and doing so "by default" might lead to some painful/scary initial upgrades.

Such change to the default could be something we do in a major release, but I think we trying hard to not do one of those any time soon, and I'm less confortable doing it in a minor release (but open to other opinions).

That said, even if we keep the current default, I'm again very on board with adding ordering options. That does imply adding some option to the compose method though, and begs the question of how such options gets passed down to that method by its existing callers.

id-ilych · 2022-09-06T11:18:15Z

Hello @pcmanus 👋 What do you think about adding a separate method shallowOrderPrintedDefinitions with an "old" behavior and use it in the composite by default, so that default behavior is preserved?

pcmanus · 2022-09-22T15:30:36Z

First, sorry for the delay in my answers here.

Since my last comment, I realised that things weren't exactly as I though they were around this issue. Let me explain.

I "though" that the ordering composition is using in federation 2 (which orders types/directives but fields within types) was the same one that in federation 1 and the one composition had always used. Which is why I was a bit worried about changing it now.

Fwiw, that's why I initially didn't understood why you said that "on major upgrades creates quite chaotic diff".

Anyway, the reason I held that belief is that I implemented the order for federation 2 and when I did so, I very much was careful to match federation 1 behaviour (I probably wouldn't have sorted only some of the elements otherwise).

But apparently there was a communication issue somewhere a year ago, because I missed that #824 was committed to federation 1 to change the ordering so everything gets sorted. This was unfortunately done after the work on federation 2 had started, and I never realised the behaviour had changed in federation 1.

Tl;dr, the fact that the ordering changed in the major upgrade between federation 1 and federation 2 is very much an oversight/accident. Not sure how that could have slipped all this time but well ...

Anyway, this does change a bit my position here. Because:

I do agree that the current ordering in federation 2 is inconsistent.
I probably overestimated before how big of a deal it is to change the ordering in a non-major release since, well, we did it with feat(federation): Deterministically sort composed schema #824 and while I'm sure it was noticed, there also wasn't too much complaints.
we did inadvertently change the ordering in federation 2 upgrades.

So I think my new preferred position here is to decide what ordering we actually want long term (at least by default), and switch to that even if it mean one more change of ordering, but then make sure we're better at keeping that consistent moving forward.

Long story short, maybe we just get back to your original suggestion @id-ilych after all, but I'd like to consult a few people that may opinions on this before completely decided here.

What do you think about adding a separate method shallowOrderPrintedDefinitions with an "old" behaviour

Whatever we end up deciding of the default, I'm totally ok with adding more methods that implement various option in the code, so +1 on that part.

id-ilych · 2022-11-21T13:02:06Z

Hello @pcmanus 👋 Any updates on that?

id-ilych · 2022-12-07T10:11:36Z

Hello @benweatherman @pcmanus 👋 A friendly reminder about pending PR.

id-ilych · 2022-12-23T09:37:58Z

Hello @benweatherman @pcmanus 👋 A friendly reminder about pending PR.

benweatherman · 2022-12-27T20:29:05Z

howdy @id-ilych! Unfortunately I no longer have the commit-bit privileges for this repo, so someone within the apollo team will need to weigh in on getting this merged.

From what I recall before I left: This is a good change, to be sure. I imagine the apollo team is wrestling with which version in which to release this type of change. It's valuable, but could cause a lot of downstream headaches looking at schema diffs that might only be reordering things.

@jeffjakub might also be able to help with this

id-ilych · 2023-01-10T08:11:38Z

Hello @jeffjakub 👋 Can you please take a look at clarify the status? What needs to be done from my side to get this merged? This PR is stuck for 5 months already :(

pcmanus · 2023-01-17T08:47:58Z

I'm very sorry on the progress on this issue. I certainly understand that this is frustrating.

To be transparent, I've been having a bit of analysis paralysis on the question of whether we should change the default composition ordering, and if yes, to what. I was hoping to get some larger conversation going on that question to gather some consensus, but didn't got much engagement so far, either because it's not very top of mind for most folks or because the status quo is good enough (but I'll admit too that I have had other priorities myself and I haven't pushed that much).

That said, I do realise that the original ask was only to add more options in the code, and not so much about changing the default for composition, and it's entirely on me for distracting from that original purpose.

Anyway, I'm still iffy about changing the default ordering of composition at the moment (until some greater consensus emerges about that question), but maybe we can just go back to your earlier suggestions of:

What do you think about adding a separate method shallowOrderPrintedDefinitions with an "old" behavior and use it in the composite by default, so that default behavior is preserved?

If the patch is updated to do so, with no change to the default behaviour, then I'll commit to getting it in quickly.

Again, my apologies for my poor handling of this issue.

changeset-bot · 2023-04-08T06:23:04Z

🦋 Changeset detected

Latest commit: d25fa15

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages

Name	Type
@apollo/composition	Patch
@apollo/federation-internals	Patch
@apollo/gateway	Patch
@apollo/query-planner	Patch
@apollo/query-graphs	Patch
@apollo/subgraph	Patch
apollo-federation-integration-testsuite	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

id-ilych · 2023-04-08T06:26:16Z

composition-js/src/compose.ts

@@ -30,7 +31,15 @@ export interface CompositionSuccess {
  errors?: undefined;
 }

-export function compose(subgraphs: Subgraphs): CompositionResult {
+export interface ComposeOptions {


I would prefer to call it CompositionOptions but this name is already exported from merge :\

I decided not to rename the existing interface to use it for the new thing to avoid breaking somebody's code in a confusing way: same name, totally different usage and meaning.

Yeah. As the TODO on CompositionOptions in merge kind of suggests, the idea was to eventually move the version from merge into compose instead of having 2 different ones. So I'd prefer doing this: move the CompositionOptions to compose.ts, use it here, and pass it down to mergeSubgraphs.

id-ilych · 2023-04-08T06:33:11Z

Hey @pcmanus 👋

Please check if that's what you meant.

pcmanus

Thank you, this looks great. I left a small comment regarding ComposeOptions, and we'll need to rollback the changelog manual edits, but happy to commit after that (and happy to deal with those minor "administrative" details myself if you prefer, just let me know)

pcmanus · 2023-04-11T16:19:18Z

composition-js/src/compose.ts

@@ -30,7 +31,15 @@ export interface CompositionSuccess {
  errors?: undefined;
 }

-export function compose(subgraphs: Subgraphs): CompositionResult {
+export interface ComposeOptions {


Yeah. As the TODO on CompositionOptions in merge kind of suggests, the idea was to eventually move the version from merge into compose instead of having 2 different ones. So I'd prefer doing this: move the CompositionOptions to compose.ts, use it here, and pass it down to mergeSubgraphs.

pcmanus · 2023-04-11T16:28:28Z

composition-js/CHANGELOG.md

@@ -1,5 +1,7 @@
 # CHANGELOG for `@apollo/composition`

+- `compose` and `composeServices` now take optional `ComposeOptions` to customize (at the moment, only `sdlPrintOptions` is supported)


We're not editing changelogs manually anymore, we use changeset instead. You can try using npx changeset add yourself, or I can do it for you. But we'll want to remove the manual edits as they'll conflict with that new process.

id-ilych · 2023-04-11T17:25:11Z

Hello @pcmanus 👋

I am not sure I fully got your idea about options. Would you mind finishing it on your own to avoid communication overhead?

This reverts commit 62581900cc66b4542457695a9246a9fd48e990e9.

pcmanus · 2023-04-12T09:10:01Z

Of course. I've pushed a few commits to the branch (and took the liberty to rebase on main). The 2 first one just remove the changelog entries and add a changesets one instead. The last one is what I meant for the composition options. I did took the liberty of switching the printing option from a function ((PrintOptions) => PrintOptions) to just the option object (PrintOptions), as this felt a tad more logical to me.

@id-ilych: I'll let you have a quick look to make sure those changes work for you. I'll merge if that's that case.

This reverts commit eb27b07.

id-ilych · 2023-04-12T10:40:58Z

@pcmanus Great! Thank you :)

id-ilych force-pushed the add-more-printSchema-ordering-options branch from 901b1f1 to 92facb1 Compare August 5, 2022 21:13

benweatherman requested a review from pcmanus August 10, 2022 16:22

id-ilych force-pushed the add-more-printSchema-ordering-options branch from b056522 to b5f3566 Compare April 8, 2023 06:23

id-ilych requested a review from a team as a code owner April 8, 2023 06:23

id-ilych force-pushed the add-more-printSchema-ordering-options branch from b5f3566 to bcd7a07 Compare April 8, 2023 06:23

id-ilych commented Apr 8, 2023

View reviewed changes

pcmanus reviewed Apr 11, 2023

View reviewed changes

pcmanus force-pushed the add-more-printSchema-ordering-options branch from aefcba4 to fb40f95 Compare April 12, 2023 09:05

id-ilych added 3 commits April 12, 2023 11:06

Add more ordering options for printSchema

a838218

Consider sub-elements in orderPrintedDefinitions

2ef7d18

Update CHANGELOG.md for composition-js and internals-js

eb27b07

benweatherman and others added 5 commits April 12, 2023 11:06

Empty commit to trigger Circle build process

888fc9c

Fallback to conservative approach

0dc009d

Update CHANGELOG.md

d1b28f2

Revert "Update CHANGELOG.md"

cc0265a

This reverts commit 62581900cc66b4542457695a9246a9fd48e990e9.

Add changesets file

ac039c5

pcmanus force-pushed the add-more-printSchema-ordering-options branch from fb40f95 to f9fc352 Compare April 12, 2023 09:06

merge ComposeOptions and CompositionOptions

3f82ecb

pcmanus force-pushed the add-more-printSchema-ordering-options branch from f9fc352 to 3f82ecb Compare April 12, 2023 09:07

Revert "Update CHANGELOG.md for composition-js and internals-js"

d25fa15

This reverts commit eb27b07.

pcmanus approved these changes Apr 12, 2023

View reviewed changes

pcmanus merged commit 2c37050 into apollographql:main Apr 12, 2023

github-actions bot mentioned this pull request Apr 12, 2023

release: on branch main #2525

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more print schema ordering options #2042

Add more print schema ordering options #2042

id-ilych commented Aug 5, 2022

netlify bot commented Aug 5, 2022 •

edited

Loading

codesandbox-ci bot commented Aug 5, 2022 •

edited

Loading

benweatherman commented Aug 5, 2022

id-ilych commented Aug 6, 2022

benweatherman commented Aug 8, 2022

id-ilych commented Aug 8, 2022

benweatherman commented Aug 8, 2022

pcmanus commented Aug 17, 2022

id-ilych commented Sep 6, 2022

pcmanus commented Sep 22, 2022

id-ilych commented Nov 21, 2022

id-ilych commented Dec 7, 2022

id-ilych commented Dec 23, 2022

benweatherman commented Dec 27, 2022

id-ilych commented Jan 10, 2023

pcmanus commented Jan 17, 2023

changeset-bot bot commented Apr 8, 2023 •

edited

Loading

id-ilych Apr 8, 2023

pcmanus Apr 11, 2023

id-ilych commented Apr 8, 2023

pcmanus left a comment

pcmanus Apr 11, 2023

pcmanus Apr 11, 2023

id-ilych commented Apr 11, 2023 •

edited

Loading

pcmanus commented Apr 12, 2023 •

edited

Loading

id-ilych commented Apr 12, 2023

		@@ -1,5 +1,7 @@
		# CHANGELOG for `@apollo/composition`

		- `compose` and `composeServices` now take optional `ComposeOptions` to customize (at the moment, only `sdlPrintOptions` is supported)

Add more print schema ordering options #2042

Add more print schema ordering options #2042

Conversation

id-ilych commented Aug 5, 2022

netlify bot commented Aug 5, 2022 • edited Loading

👷 Deploy request for apollo-federation-docs pending review.

codesandbox-ci bot commented Aug 5, 2022 • edited Loading

benweatherman commented Aug 5, 2022

id-ilych commented Aug 6, 2022

benweatherman commented Aug 8, 2022

id-ilych commented Aug 8, 2022

benweatherman commented Aug 8, 2022

pcmanus commented Aug 17, 2022

id-ilych commented Sep 6, 2022

pcmanus commented Sep 22, 2022

id-ilych commented Nov 21, 2022

id-ilych commented Dec 7, 2022

id-ilych commented Dec 23, 2022

benweatherman commented Dec 27, 2022

id-ilych commented Jan 10, 2023

pcmanus commented Jan 17, 2023

changeset-bot bot commented Apr 8, 2023 • edited Loading

🦋 Changeset detected

id-ilych Apr 8, 2023

Choose a reason for hiding this comment

pcmanus Apr 11, 2023

Choose a reason for hiding this comment

id-ilych commented Apr 8, 2023

pcmanus left a comment

Choose a reason for hiding this comment

pcmanus Apr 11, 2023

Choose a reason for hiding this comment

pcmanus Apr 11, 2023

Choose a reason for hiding this comment

id-ilych commented Apr 11, 2023 • edited Loading

pcmanus commented Apr 12, 2023 • edited Loading

id-ilych commented Apr 12, 2023

netlify bot commented Aug 5, 2022 •

edited

Loading

codesandbox-ci bot commented Aug 5, 2022 •

edited

Loading

changeset-bot bot commented Apr 8, 2023 •

edited

Loading

id-ilych commented Apr 11, 2023 •

edited

Loading

pcmanus commented Apr 12, 2023 •

edited

Loading